MovableMan - fix usage of unordered_set::erase #226
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`unordered_set::erase` invalidates the pointer that was erased. https://en.cppreference.com/w/cpp/container/unordered_set/erase.html > References and iterators to the erased elements are invalidated. Other iterators and references are not invalidated. I also tested with a fresh C++ code file with `-fsanitize-address` and, indeed, the pattern of code of using an iterator variable to erase and then accessing through it reliably explodes. --- What prompted this is that I have just had a completely random segfault crash originating from `SDL_Scancode ()` (????????) while playing the game. Here's the gdb backtrace (yes i play this game in debug build in gdb at this point lmao): ``` (gdb) bt #0 0x0000555556b39558 in typeinfo name for SDL_Scancode () cortex-command-community#1 0x0000555555ae47aa in RTE::MovableMan::Update (this=0x555557453f50) at ../Source/Managers/MovableMan.cpp:1618 cortex-command-community#2 0x000055555567597c in RunGameLoop () at ../SourceMain.cpp:354 cortex-command-community#3 0x000055555567658a in main (argc=1 argv=0x7fffffffdc88) at ../Source/Main.cpp:468 ``` The last sensible location in that backtrace is right after the erase. Nothing seems to be broken with this applied. --- There are comments and code around these spots that this commit touches that are apparently to work around the same issue I experienced. They're probably redundant/unneeded any more lol
Contributor
|
Feels dumb I didn't catch this. Nice one! |
Causeless
merged commit 066841d
into
cortex-command-community:development
0 of 4 checks passed
|
I just had a crash: Is it related to this change? |
Contributor
Author
Unlikely I think. Line 1617 is The game does keep having random rare memory corruption crashes in most random of places for me, and when I ran the game with gcc's undefined behavior analyzer, it kept spewing quite a bunch of various runtime errors related to things being wrong classes or something... I think the codebase is just kind of a swiss cheese at the moment with undefined behavior, and that inevitably bites. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
unordered_set::eraseinvalidates the pointer that was erased.https://en.cppreference.com/w/cpp/container/unordered_set/erase.html
I also tested with a fresh C++ code file with
-fsanitize-addressand, indeed, the pattern of code of using an iterator variable to erase and then accessing through it reliably explodes.What prompted this is that I have just had a completely random segfault crash originating from
SDL_Scancode ()(????????) while playing the game. Here's the gdb backtrace (yes i play this game in debug build in gdb at this point lmao):The last sensible location in that backtrace is right after the erase.
Nothing seems to be broken with this applied.
There are comments and code around these spots that this commit touches that are apparently to work around the same issue I experienced. They're probably redundant/unneeded any more lol